Fix Decimal type when precision and scale are unspecified #197
Conversation
| # TODO(#178) this should never use DecimalWithoutScale since scale | ||
| # is assumed to be 0 if it is not explicitly defined. | ||
| # |
There was a problem hiding this comment.
I think this comment should still be here since ideally we wouldn't need this conditional and OID::Decimal would properly handle the case where precision is defined, but the scale is not.
There was a problem hiding this comment.
Yeah, I removed it because it's not in the original rails code and the new logic is much closer to the original logic in rails.
There was a problem hiding this comment.
Yes, you're right. I think issue #178 can be closed since this handles the case described in that issue (DECIMAL where only precision is specified).
There was a problem hiding this comment.
Actually, can you add this back in now? I'm going to add to that issue to reflect the problem we saw with precision being improperly handled.
There was a problem hiding this comment.
I verified it does the same behavior in vanilla activerecord.
| setup do | ||
| @connection = ActiveRecord::Base.connection | ||
| @connection.create_table("postgresql_numbers", force: true) do |t| | ||
| t.decimal 'decimal_default' | ||
| t.decimal 'decimal_with_p', precision: 5 | ||
| t.decimal 'decimal_with_p_and_s', precision: 5, scale: 4 | ||
| end | ||
| end |
There was a problem hiding this comment.
Looks like the indentation's off here.
| record = PostgresqlNumber.new(decimal_default: 111.222, decimal_with_p: 111.222, decimal_with_p_and_s: 111.222) | ||
| assert_equal record.decimal_default, 111.222 | ||
| assert_equal record.decimal_with_p, 11 |
There was a problem hiding this comment.
Looks like this is failing because the ActiveRecord Types don't handle precision properly. Both Type::DecimalWithoutScale and OID::Decimal allow any number of digits to the left of the decimal point for some reason. I can look into it more to see if using the limit parameter helps.
There was a problem hiding this comment.
I can just drop that test since it's not as important, it's really only the first assert that I'm actually worried about here.
|
I think this is good now. I'll add to #178 to reflect the issue where precision is not handled correctly by the ActiveRecord types and we'll keep that comment in there to track it. @rafiss what are your thoughts on this? This patch fixes the case where a decimal column is created without specifying precision or scale, but there are still inconsistencies between ActiveRecord's decimal implementation and CockroachDBs implementation. We talked about this before and the conclusion was to keep everything consistent with ActiveRecord and just track it in case there are changes. Also once the record is created in the database, it should be properly handled and subsequent reads from the db will have the correct precision and scale, but as we saw from the failing test it does create issues before then. |
|
@keithdoggett thanks for reviewing and your explanation! @carlallen would you mind rebasing on top of the latest changes from master and squashing this into one commit with a proper message? then i will merge this |
carlallen
changed the title
|
Rebased and squashed with a more informative message |
|
Thanks for finding this issue and fixing it @carlallen! Just waiting for CI to pass, then I'll merge |
The changes to make intervals work broke decimal fields that do not specify precision and scale. #176